Make OME Deployment Flexible with GPU only option; Improve Build Cache and Docker Build #310
Conversation
Summary of ChangesHello @Kangyan-Zhou, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new capability to deploy the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Kangyan-Zhou
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two main improvements: flexible deployment of the model agent to GPU nodes and enhanced build caching for Docker and Go. The changes to the Helm charts add valuable flexibility for GPU-specific deployments. The Dockerfile and Makefile modifications significantly improve build times by leveraging caching, which is a great enhancement for the development workflow. I've found a critical issue in the Helm template logic that needs to be addressed, along with a couple of medium-severity suggestions for improving the Makefile and error handling.
| containers: | ||
| - name: model-agent | ||
| image: {{ include "ome.imageWithHub" (dict "values" .Values "repository" .Values.modelAgent.image.repository "tag" .Values.modelAgent.image.tag) }} | ||
| image: {{ include "ome.imageWithHub" (dict "values" (merge (dict "global" (dict "hub" (default .Values.global.hub .Values.modelAgent.image.hub))) .Values) "repository" .Values.modelAgent.image.repository "tag" .Values.modelAgent.image.tag) }} |
There was a problem hiding this comment.
This logic for overriding the image hub appears to have two issues:
- The
defaultfunction arguments are swapped. It should be(default .Values.modelAgent.image.hub .Values.global.hub)to prioritize the model-agent specific hub, as documented invalues.yaml. - The
mergefunction arguments are also swapped. Helm'smergegives precedence to the right-most dictionary, so.Valuesis currently overwriting your hub override. The arguments should be swapped to merge the override into.Values.
Here is the corrected line:
image: {{ include "ome.imageWithHub" (dict "values" (merge .Values (dict "global" (dict "hub" (default .Values.modelAgent.image.hub .Values.global.hub)))) "repository" .Values.modelAgent.image.repository "tag" .Values.modelAgent.image.tag) }}| export GOMODCACHE | ||
|
|
||
| # Ensure cache directories exist | ||
| $(shell mkdir -p $(GOCACHE) $(GOMODCACHE)) |
There was a problem hiding this comment.
The $(shell mkdir -p ...) command is executed every time make is invoked, during the parsing phase. While mkdir -p is idempotent, this is generally not a good practice as it can have side effects and is not explicit. It's better to create directories as part of a target's recipe or as a prerequisite for targets that need them. For example, you could create a .PHONY setup target and add it as a dependency to your build targets.
| if totalErrors > 0 { | ||
| // Collect error details | ||
| var errorFiles []string | ||
| var errorMessages []string |
There was a problem hiding this comment.
You've added errorMessages to collect more detailed errors, which is a great improvement for debuggability. However, this new slice is populated but never used later in the function. The function still prints the less informative errorFiles slice and returns a generic error message. Consider using errorMessages to provide more detailed feedback to the user on failure, for example by joining them into the returned error string or logging them.
Kangyan-Zhou
changed the title
1 participant
Add an option to only deploy model agent to gpu nodes so that if we won't waste resource if we don't have CPU serving.
Add option to use Ubuntu 22.04 instead of the default Oracle Linux.
Add cache for build to reduce build time across the board, especially xnet.